-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pine: Compute the squared norm just once #649
Conversation
fafa62f
to
7f62e60
Compare
7f62e60
to
08aca1a
Compare
Split the existing FLP circuit into two: 1. One that computes the squared norm bound and checks that it is equal to the claimed value 2. Another for everything else, including checking that the claimed norm bound is in range and that the wraparound tests succeeded The first circuit does not require joint randomness to compute, which means we can safely run it once without opening ourselves to offline attacks. It is also the most expensive part of the computation. This change aligns our implementation with a planned change for the next version of PINE: junyechen1996/draft-chen-cfrg-vdaf-pine#92 junyechen1996/draft-chen-cfrg-vdaf-pine#94
08aca1a
to
493b6de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to approve to unblock, but maybe prio should change to have more smaller traits to avoid situations like this? I'm not super familiar with prio but methods like
fn encode_measurement(&self, _measurement: &()) -> Result<Vec<F>, FlpError>
fn decode_result(&self, _data: &[F], _num_measurements: usize) -> Result<(), FlpError>
fn truncate(&self, _input: Vec<F>) -> Result<Vec<F>, FlpError>
fn output_len(&self) -> usize
seem to be concerned with the domain of serialization(?) whereas the other methods of the trait seem concerned cryptography itself. Maybe these could be split into two traits?
I agree, see #649 (comment). Before merging this I want @armfazh to review :) |
.chunks(self.cfg.chunk_len_sq_norm_equal) | ||
.map(|chunk| { | ||
for x in chunk { | ||
buf.push(*x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double-check: push twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this is correct! buf
is a sequence of inputs to the Mul
gadget, which has two input wires and outputs the multiplication of the inputs. Here we're just using the Mul
gadget to square each of the gradient entries.
if !self.flp.decide(verifier)? { | ||
return Err(VdafError::Uncategorized("proof check failed".into())); | ||
return Err(VdafError::Uncategorized("main proof check failed".into())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it maybe good to refactor the 'main' proof and give it a more specific name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the functional split between the two FLPs is a little awkward. The "main" circuit is everything but the squared norm equality check. In the future we might try to go for a split for which the two circuits would have more meaningful names: junyechen1996/draft-chen-cfrg-vdaf-pine#95
Based on #648 (merge that first).
Partially addresses #618.
Split the existing FLP circuit into two:
One that computes the squared norm bound and checks that it is equal to the claimed value
Another for everything else, including checking that the claimed norm bound is in range and that the wraparound tests succeeded
The first circuit does not require joint randomness to compute, which means we can safely run it once without opening ourselves to offline attacks. It is also the most expensive part of the computation.
This change aligns our implementation with a planned change for the next version of PINE:
junyechen1996/draft-chen-cfrg-vdaf-pine#92
junyechen1996/draft-chen-cfrg-vdaf-pine#94